Skip to content

Fix exception handling in NextcloudClient#394

Merged
tobiasKaminsky merged 1 commit into
masterfrom
ezaquarii/fix-nextcloud-client-exception-handling
Feb 7, 2020
Merged

Fix exception handling in NextcloudClient#394
tobiasKaminsky merged 1 commit into
masterfrom
ezaquarii/fix-nextcloud-client-exception-handling

Conversation

@ezaquarii

@ezaquarii ezaquarii commented Feb 3, 2020 •
edited by AndyScherzinger
Loading

Copy link
Copy Markdown
Contributor

ezaquarii
ezaquarii commented Feb 3, 2020
View reviewed changes
Comment thread src/main/java/com/nextcloud/common/NextcloudClient.kt Outdated
return method.execute(this)
}

@Throws(IOException::class)

@ezaquarii ezaquarii Feb 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasKaminsky This will proably break clients, but since IOException is checked, it must be handled.
There is no API to return valid Response in case of error, as OkHttp3 use exceptions to signal abnormal condition. We should follow.

@tobiasKaminsky tobiasKaminsky Feb 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I would go instead with try/catch so that we can return our own error code.
After all, this is in our library and also should only be called within the library.

@ezaquarii ezaquarii Feb 4, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrapped the exception in a union-like object.

@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 6276eef to 31a41df Compare February 3, 2020 22:35
ezaquarii
ezaquarii commented Feb 3, 2020
View reviewed changes
Comment thread src/main/java/com/nextcloud/common/NextcloudClient.kt

class NextcloudClient(var baseUri: Uri, val context: Context) {
class NextcloudClient(var baseUri: Uri, val context: Context, client: OkHttpClient) {
lateinit var credentials: String

@ezaquarii ezaquarii Feb 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misused lateinit. This should be initialized here. I'll open another ticket for this, tho.

@tobiasKaminsky tobiasKaminsky Feb 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed via #396

@ezaquarii

ezaquarii commented Feb 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@tobiasKaminsky Could you pealse check the method annotated with @Throws? I won't have bandwidth to do it now. I guess some client must be fixed to handle the IOException.

@codecov

codecov Bot commented Feb 3, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #394 into master will increase coverage by 0.24%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage      37%   37.24%   +0.24%     
==========================================
  Files         133      134       +1     
  Lines        5737     5751      +14     
  Branches      751      751              
==========================================
+ Hits         2123     2142      +19     
+ Misses       3289     3285       -4     
+ Partials      325      324       -1
Impacted Files Coverage Δ
.../main/java/com/nextcloud/common/NextcloudClient.kt 50% <100%> (+11.11%) ⬆️
.../main/java/com/nextcloud/common/ResponseOrError.kt 100% <100%> (ø)
...main/java/com/nextcloud/common/OkHttpMethodBase.kt 57.14% <62.5%> (+4.01%) ⬆️
...d/lib/common/operations/RemoteOperationResult.java 42.07% <0%> (+0.3%) ⬆️
...wncloud/android/lib/resources/files/FileUtils.java 85.71% <0%> (+14.28%) ⬆️

@tobiasKaminsky tobiasKaminsky mentioned this pull request Feb 4, 2020
Merged
@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 31a41df to 78ec45a Compare February 4, 2020 23:11
@AndyScherzinger AndyScherzinger mentioned this pull request Feb 5, 2020
Closed
tobiasKaminsky
tobiasKaminsky requested changes Feb 5, 2020
View reviewed changes

@tobiasKaminsky tobiasKaminsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract ResponseOrError

Comment thread src/main/java/com/nextcloud/common/NextcloudClient.kt Outdated
val client: OkHttpClient
val client = client

data class ResponseOrError private constructor(val result: Response?, val error: Exception?) {

@tobiasKaminsky tobiasKaminsky Feb 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach 👍
To not have too many things in one class, can we extract this into its own class?

@ezaquarii ezaquarii Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracted

@nextcloud nextcloud deleted a comment from ezaquarii Feb 5, 2020
@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 78ec45a to eb51e04 Compare February 5, 2020 22:14
@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from eb51e04 to 220afbb Compare February 6, 2020 06:31
@ezaquarii

ezaquarii commented Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

@tobiasKaminsky I made the okhttp3 execute() request internal. Please confirm if this is what we can do, ie. we don't plan to use it outside the library.

@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 220afbb to 803df96 Compare February 6, 2020 16:18
@nextcloud-android-bot

nextcloud-android-bot commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator

Lint

TypemasterPR
Warnings00
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

@tobiasKaminsky

tobiasKaminsky commented Feb 7, 2020

Copy link
Copy Markdown
Member

@tobiasKaminsky I made the okhttp3 execute() request internal. Please confirm if this is what we can do, ie. we don't plan to use it outside the library.

Should be fine, I currently cannot imagine why we need this outside our lib, but we can still change it later, if required.

@tobiasKaminsky
tobiasKaminsky merged commit e8f50d6 into master Feb 7, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the ezaquarii/fix-nextcloud-client-exception-handling branch February 7, 2020 07:53
This was referenced Feb 9, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobiasKaminsky tobiasKaminsky tobiasKaminsky approved these changes

Assignees

No one assigned

Labels

3. to review

Projects

None yet

Milestone

No milestone

3 participants

@ezaquarii @nextcloud-android-bot @tobiasKaminsky